Skip to content

Conversation

@sgrekhov
Copy link
Contributor

In case of final variables there is an error that the variable is already initialized and this error may shadow the tested one. But I don't see another way how to test the initializer list behavior.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with a handful of comments.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Updated. Not all of recommendations implemented as suggested. PTAL.

@sgrekhov sgrekhov requested a review from eernstg October 15, 2025 11:23
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the treatment of super should indeed be tested, but it doesn't seem to fit into the context where every other test is testing the treatment of an instance variable which is being initialized more than once.

However, don't we already have some tests where the treatment of super is handled? Perhaps there are some missing cases in those tests?

Perhaps it's possible to find a test that tests the treatment of super parameters (no declaring constructors involved at all), and then modify those test cases such that the super parameter is introduced by a primary constructor rather than a regular (non-declaring) one?

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed superparameters tests. I'll add them in a separate PR (probably as an additional test for superparameters).

@sgrekhov sgrekhov requested a review from eernstg October 15, 2025 13:59
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Sorry about the nitpicking!

@eernstg eernstg merged commit 137d34b into dart-lang:master Oct 15, 2025
2 checks passed
@sgrekhov
Copy link
Contributor Author

sgrekhov commented Oct 16, 2025

LGTM, thanks! Sorry about the nitpicking!

No problem at all! Thank you!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 17, 2025
2025-10-16 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for super parameters (dart-lang/co19#3352)
2025-10-16 sgrekhov22@gmail.com dart-lang/co19#3315. Add more declaring constructors tests (dart-lang/co19#3351)
2025-10-15 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for initializer list (dart-lang/co19#3349)
2025-10-15 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for the `new` keyword (dart-lang/co19#3350)
2025-10-14 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for formal parameters. Part 5. (dart-lang/co19#3348)
2025-10-13 sgrekhov22@gmail.com dart-lang/co19#3315. Update assertions. Add test for setters. (dart-lang/co19#3347)
2025-10-10 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for formal parameters. Part 4. (dart-lang/co19#3346)
2025-10-10 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for formal parameters. Part 3. (dart-lang/co19#3345)
2025-10-10 sgrekhov22@gmail.com dart-lang/co19#3315. Add tests for formal parameters. Part 2. (dart-lang/co19#3344)

R=athom@google.com, eernst@google.com

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,dart2js-minified-linux-d8-try
Change-Id: Ie4824457e4c1843e8406dbb7ba8c0a6a10728410
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/455600
Auto-Submit: Sergey Grekhov <sgrekhov22@gmail.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Alexander Thomas <athom@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants